Conversation
Summary of ChangesHello @MikeeBuilds, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the ClawPinch security scanner by implementing a robust finding suppression mechanism. Users can now define accepted risks and temporary exceptions in a .clawpinch-ignore.json file, preventing these findings from causing CI/CD pipeline failures or cluttering active reports. The system intelligently handles expiration dates for suppressions and provides clear visibility options through new command-line flags, allowing for both focused active finding reviews and full audit scans. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a robust finding suppression mechanism, which is a great addition for managing security findings in different environments. The implementation is comprehensive, covering command-line flags, configuration, filtering logic, and reporting. The documentation updates in README.md, CLAUDE.md, and SKILL.md are thorough and clear. I've identified a few areas for improvement, including a bug in how the ignore file path is resolved, opportunities to enhance performance in the core filtering logic, and some code duplication. My suggestions aim to fix these issues and improve the maintainability of the new code.
clawpinch.sh
Outdated
| if [[ -n "$OPENCLAW_CONFIG" ]] && [[ -f "$OPENCLAW_CONFIG/.clawpinch-ignore.json" ]]; then | ||
| ignore_file="$OPENCLAW_CONFIG/.clawpinch-ignore.json" |
There was a problem hiding this comment.
The logic to locate the .clawpinch-ignore.json file incorrectly treats $OPENCLAW_CONFIG as a directory path. The get_openclaw_config function returns a path to a file (e.g., ~/.config/openclaw/openclaw.json), so attempting to append /.clawpinch-ignore.json to it will result in an invalid path and fail to find the ignore file. You should use dirname to extract the directory from the config file path.
| if [[ -n "$OPENCLAW_CONFIG" ]] && [[ -f "$OPENCLAW_CONFIG/.clawpinch-ignore.json" ]]; then | |
| ignore_file="$OPENCLAW_CONFIG/.clawpinch-ignore.json" | |
| if [[ -n "$OPENCLAW_CONFIG" ]] && [[ -f "$(dirname "$OPENCLAW_CONFIG")/.clawpinch-ignore.json" ]]; then | |
| ignore_file="$(dirname "$OPENCLAW_CONFIG")/.clawpinch-ignore.json" |
clawpinch.sh
Outdated
| if [[ "$SHOW_SUPPRESSED" -eq 1 ]]; then | ||
| # Mark suppressed findings with a "suppressed": true field before merging | ||
| MARKED_SUPPRESSED="$(echo "$SUPPRESSED_FINDINGS" | jq '[.[] | . + {suppressed: true}]')" | ||
| DISPLAY_FINDINGS="$(echo "$ACTIVE_FINDINGS" "$MARKED_SUPPRESSED" | jq -s '.[0] + .[1] | sort_by(.severity | if . == "critical" then 0 elif . == "warn" then 1 elif . == "info" then 2 elif . == "ok" then 3 else 4 end)')" |
There was a problem hiding this comment.
This jq command duplicates the sorting logic that is defined earlier in the script as the sev_order function. To improve maintainability and avoid potential inconsistencies in the future, it's better to define and reuse the sorting function here as well, similar to how it's done for SORTED_FINDINGS.
| DISPLAY_FINDINGS="$(echo "$ACTIVE_FINDINGS" "$MARKED_SUPPRESSED" | jq -s '.[0] + .[1] | sort_by(.severity | if . == "critical" then 0 elif . == "warn" then 1 elif . == "info" then 2 elif . == "ok" then 3 else 4 end)')" | |
| DISPLAY_FINDINGS="$(echo "$ACTIVE_FINDINGS" "$MARKED_SUPPRESSED" | jq -s 'def sev_order: if . == \"critical\" then 0 elif . == \"warn\" then 1 elif . == \"info\" then 2 elif . == \"ok\" then 3 else 4 end; .[0] + .[1] | sort_by(.severity | sev_order)')" |
scripts/helpers/suppression.sh
Outdated
| if command -v date &>/dev/null; then | ||
| # Try to get ISO 8601 timestamp (works on GNU date and macOS date) | ||
| if date -u +"%Y-%m-%dT%H:%M:%SZ" &>/dev/null; then | ||
| now="$(date -u +"%Y-%m-%dT%H:%M:%SZ")" | ||
| else | ||
| # Fallback if date format fails | ||
| now="" | ||
| fi | ||
| else | ||
| now="" | ||
| fi |
There was a problem hiding this comment.
The date command is called multiple times here: once to check for its existence, once to test the format, and a final time to get the value. This can be simplified and made more efficient by attempting to capture the output directly and checking the exit code. This avoids redundant process calls.
| if command -v date &>/dev/null; then | |
| # Try to get ISO 8601 timestamp (works on GNU date and macOS date) | |
| if date -u +"%Y-%m-%dT%H:%M:%SZ" &>/dev/null; then | |
| now="$(date -u +"%Y-%m-%dT%H:%M:%SZ")" | |
| else | |
| # Fallback if date format fails | |
| now="" | |
| fi | |
| else | |
| now="" | |
| fi | |
| if command -v date &>/dev/null; then | |
| # Try to get ISO 8601 timestamp (works on GNU date and macOS date) | |
| if ! now="$(date -u +'%Y-%m-%dT%H:%M:%SZ' 2>/dev/null)"; then | |
| # Fallback if date format fails | |
| now="" | |
| fi | |
| else | |
| now="" | |
| fi |
scripts/helpers/suppression.sh
Outdated
| local now | ||
| if command -v date &>/dev/null && date -u +"%Y-%m-%dT%H:%M:%SZ" &>/dev/null; then | ||
| now="$(date -u +"%Y-%m-%dT%H:%M:%SZ")" | ||
| else | ||
| now="" | ||
| fi |
There was a problem hiding this comment.
Similar to the is_suppressed function, the date command is called multiple times to get the current timestamp. This can be optimized by capturing the output and checking the exit code in a single step.
| local now | |
| if command -v date &>/dev/null && date -u +"%Y-%m-%dT%H:%M:%SZ" &>/dev/null; then | |
| now="$(date -u +"%Y-%m-%dT%H:%M:%SZ")" | |
| else | |
| now="" | |
| fi | |
| local now | |
| if command -v date &>/dev/null; then | |
| if ! now="$(date -u +'%Y-%m-%dT%H:%M:%SZ' 2>/dev/null)"; then | |
| now="" | |
| fi | |
| else | |
| now="" | |
| fi |
scripts/helpers/suppression.sh
Outdated
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" '{ | ||
| active: map( | ||
| . as $finding | | ||
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | ||
| if $suppression then | ||
| if $suppression.expires then | ||
| if $suppression.expires <= $now then $finding else empty end | ||
| else | ||
| empty | ||
| end | ||
| else | ||
| $finding | ||
| end | ||
| ), | ||
| suppressed: map( | ||
| . as $finding | | ||
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | ||
| if $suppression then | ||
| if $suppression.expires then | ||
| if $suppression.expires > $now then $finding else empty end | ||
| else | ||
| $finding | ||
| end | ||
| else | ||
| empty | ||
| end | ||
| ) | ||
| }' |
There was a problem hiding this comment.
The current jq logic for filtering findings is inefficient. It iterates over all findings and, for each one, iterates over all suppression rules, leading to O(N*M) complexity. This can be significantly optimized by first converting the suppressions array into a map for O(1) lookups, and then processing the findings in a single pass using reduce. This change also adds the suppression details to the finding object in the output, which aligns the implementation with the documentation in README.md.
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" '{ | |
| active: map( | |
| . as $finding | | |
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | |
| if $suppression then | |
| if $suppression.expires then | |
| if $suppression.expires <= $now then $finding else empty end | |
| else | |
| empty | |
| end | |
| else | |
| $finding | |
| end | |
| ), | |
| suppressed: map( | |
| . as $finding | | |
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | |
| if $suppression then | |
| if $suppression.expires then | |
| if $suppression.expires > $now then $finding else empty end | |
| else | |
| $finding | |
| end | |
| else | |
| empty | |
| end | |
| ) | |
| }' | |
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" ' | |
| ($suppressions | map({(.id): .}) | add) as $suppressions_map | | |
| reduce .[] as $finding ({active: [], suppressed: []}; | |
| . as $state | | |
| $suppressions_map[$finding.id] as $suppression | | |
| if $suppression then | |
| if $suppression.expires and $suppression.expires <= $now then | |
| $state | .active += [$finding] | |
| else | |
| $state | .suppressed += [$finding + {suppression: ($suppression | del(.id))}] | |
| end | |
| else | |
| $state | .active += [$finding] | |
| end | |
| ) | |
| ' |
scripts/helpers/suppression.sh
Outdated
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" '{ | ||
| active: map( | ||
| . as $finding | | ||
| if ($suppressions | map(select(.id == $finding.id)) | length == 0) then | ||
| $finding | ||
| else | ||
| empty | ||
| end | ||
| ), | ||
| suppressed: map( | ||
| . as $finding | | ||
| if ($suppressions | map(select(.id == $finding.id)) | length > 0) then | ||
| $finding | ||
| else | ||
| empty | ||
| end | ||
| ) | ||
| }' |
There was a problem hiding this comment.
This block has the same performance issue as the one above. It can be optimized by using a jq map for suppressions and reduce to process findings in a single pass. This will improve performance and also add suppression details to the output for consistency.
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" '{ | |
| active: map( | |
| . as $finding | | |
| if ($suppressions | map(select(.id == $finding.id)) | length == 0) then | |
| $finding | |
| else | |
| empty | |
| end | |
| ), | |
| suppressed: map( | |
| . as $finding | | |
| if ($suppressions | map(select(.id == $finding.id)) | length > 0) then | |
| $finding | |
| else | |
| empty | |
| end | |
| ) | |
| }' | |
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" ' | |
| ($suppressions | map({(.id): .}) | add) as $suppressions_map | | |
| reduce .[] as $finding ({active: [], suppressed: []}; | |
| . as $state | | |
| $suppressions_map[$finding.id] as $suppression | | |
| if $suppression then | |
| $state | .suppressed += [$finding + {suppression: ($suppression | del(.id))}] | |
| else | |
| $state | .active += [$finding] | |
| end | |
| ) | |
| ' |
Greptile OverviewGreptile SummaryThis PR adds a finding-suppression mechanism via a Main issues to address before merge are (1) the suppressed count is never displayed because Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant U as User/CI
participant CP as clawpinch.sh
participant SC as Scanner scripts
participant SUP as suppression.sh
participant UI as report.sh/interactive.sh
U->>CP: Run scan
CP->>SC: Execute scanners
SC-->>CP: Findings JSON arrays
CP->>CP: Merge + sort findings
CP->>SUP: filter_findings(ignore_file)
SUP-->>CP: active + suppressed arrays
CP->>CP: Optionally merge suppressed into display
alt JSON output
CP-->>U: {findings, suppressed}
else Terminal output
CP->>UI: Render findings + summary
UI-->>U: Terminal report
end
|
scripts/helpers/suppression.sh
Outdated
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" '{ | ||
| active: map( | ||
| . as $finding | | ||
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | ||
| if $suppression then | ||
| if $suppression.expires then | ||
| if $suppression.expires <= $now then $finding else empty end | ||
| else | ||
| empty | ||
| end | ||
| else | ||
| $finding | ||
| end | ||
| ), | ||
| suppressed: map( | ||
| . as $finding | | ||
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | ||
| if $suppression then | ||
| if $suppression.expires then | ||
| if $suppression.expires > $now then $finding else empty end | ||
| else | ||
| $finding | ||
| end | ||
| else | ||
| empty | ||
| end | ||
| ) | ||
| }' |
There was a problem hiding this comment.
Suppressed findings don't include the suppression metadata (reason, suppressed_by, etc.) in the output. According to README.md:269-274, the JSON output should include a suppression object with this metadata. Consider enriching the suppressed findings with their corresponding suppression data:
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" '{ | |
| active: map( | |
| . as $finding | | |
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | |
| if $suppression then | |
| if $suppression.expires then | |
| if $suppression.expires <= $now then $finding else empty end | |
| else | |
| empty | |
| end | |
| else | |
| $finding | |
| end | |
| ), | |
| suppressed: map( | |
| . as $finding | | |
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | |
| if $suppression then | |
| if $suppression.expires then | |
| if $suppression.expires > $now then $finding else empty end | |
| else | |
| $finding | |
| end | |
| else | |
| empty | |
| end | |
| ) | |
| }' | |
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" '{ | |
| active: map( | |
| . as $finding | | |
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | |
| if $suppression then | |
| if $suppression.expires then | |
| if $suppression.expires <= $now then $finding else empty end | |
| else | |
| empty | |
| end | |
| else | |
| $finding | |
| end | |
| ), | |
| suppressed: map( | |
| . as $finding | | |
| ($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression | | |
| if $suppression then | |
| if $suppression.expires then | |
| if $suppression.expires > $now then | |
| $finding + {suppression: {reason: $suppression.reason, expires: $suppression.expires, suppressed_by: $suppression.suppressed_by}} | |
| else empty end | |
| else | |
| $finding + {suppression: {reason: $suppression.reason, suppressed_by: $suppression.suppressed_by}} | |
| end | |
| else | |
| empty | |
| end | |
| ) | |
| }' |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/helpers/suppression.sh
Line: 159:186
Comment:
Suppressed findings don't include the suppression metadata (reason, suppressed_by, etc.) in the output. According to README.md:269-274, the JSON output should include a `suppression` object with this metadata. Consider enriching the suppressed findings with their corresponding suppression data:
```suggestion
echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" '{
active: map(
. as $finding |
($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression |
if $suppression then
if $suppression.expires then
if $suppression.expires <= $now then $finding else empty end
else
empty
end
else
$finding
end
),
suppressed: map(
. as $finding |
($suppressions | map(select(.id == $finding.id)) | .[0]) as $suppression |
if $suppression then
if $suppression.expires then
if $suppression.expires > $now then
$finding + {suppression: {reason: $suppression.reason, expires: $suppression.expires, suppressed_by: $suppression.suppressed_by}}
else empty end
else
$finding + {suppression: {reason: $suppression.reason, suppressed_by: $suppression.suppressed_by}}
end
else
empty
end
)
}'
```
How can I resolve this? If you propose a fix, please make it concise.…pressions Implemented suppression.sh helper module with three core functions: - load_suppressions(): Reads .clawpinch-ignore.json and parses suppressions array - is_suppressed(): Checks if a finding ID is currently suppressed (with expiration) - filter_findings(): Splits findings array into active and suppressed based on suppressions Features: - Graceful handling of missing or invalid JSON files - ISO 8601 expiration date support with automatic expiry checking - Fallback behavior when jq or date commands unavailable - Follows patterns from common.sh and redact.sh Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…after fi Integrated suppression.sh into the main orchestrator: - Sourced suppression.sh helper following the pattern of other helpers - Applied filter_findings() after sorting to split findings into active and suppressed - Updated JSON output to include both 'findings' and 'suppressed' arrays - Updated display logic to use DISPLAY_FINDINGS (active + suppressed if --show-suppressed) - Updated severity counts to only count active findings for exit code calculation - Updated remediation to use active findings only Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
… suppres Added suppressed count to summary dashboard: - Calculate count_suppressed from SUPPRESSED_FINDINGS array - Pass suppressed count to print_summary function - Display suppressed count in summary (only when > 0) - Line appears after timing line in dashboard Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…lude sup When --show-suppressed flag is used, suppressed findings now appear in the output with clear visual indicators: 1. clawpinch.sh: Mark suppressed findings with "suppressed": true field when merging them for display 2. scripts/helpers/interactive.sh: In print_findings_compact, check for suppressed field and add [SUPPRESSED] prefix + dim the row 3. scripts/helpers/report.sh: In print_finding and print_finding_ok, check for suppressed field and add [SUPPRESSED] prefix + dim the text Visual indicators: - [SUPPRESSED] prefix on title - Dimmed text color to distinguish from active findings Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…sion feature info Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Integration test for finding suppression feature: - Created .clawpinch-ignore.json with CHK-CFG-001 suppressed (no expiry) - Verified all 5 test scenarios pass: ✅ Finding appears in suppressed array (not findings array) ✅ Suppressed findings don't affect exit code ✅ --show-suppressed flag displays suppressed findings ✅ --no-ignore flag disables suppressions for full audit ✅ Exit code correctly reflects only active critical findings Test results documented in build-progress.txt. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…nd past) ✅ Integration test completed successfully Verified expiration behavior: - Future expiry (2030-12-31): Finding stays in suppressed array - Past expiry (2020-01-01): Finding moves back to active findings - Expiration logic correctly uses ISO 8601 timestamp comparison - Finding counts update correctly when suppressions expire Test setup: - OpenClaw config with CHK-CFG-002 trigger (groupPolicy=open) - Suppression file with expiry dates - JSON output validation All verification steps documented in build-progress.txt
bece632 to
22b5b8f
Compare
| "suppression": { | ||
| "reason": "Dev environment - reviewed by security team", | ||
| "expires": "2025-12-31T23:59:59Z", | ||
| "suppressed_by": "devops@example.com" | ||
| } |
There was a problem hiding this comment.
Documentation shows suppression object in JSON output, but filter_findings() at scripts/helpers/suppression.sh:173-185 doesn't add it - just copies findings as-is.
Either enrich findings with metadata or update docs to reflect actual output format.
Prompt To Fix With AI
This is a comment left during a code review.
Path: README.md
Line: 348:352
Comment:
Documentation shows `suppression` object in JSON output, but `filter_findings()` at `scripts/helpers/suppression.sh:173-185` doesn't add it - just copies findings as-is.
Either enrich findings with metadata or update docs to reflect actual output format.
How can I resolve this? If you propose a fix, please make it concise.…, add suppression metadata to output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All review feedback has been addressed:
@gemini-code-assist @greptile-apps — requesting re-review. Thanks! |
There was a problem hiding this comment.
Code Review
This pull request introduces a robust finding suppression feature, which is a great addition to the tool. However, a critical security concern has been identified: a potential crash (fail-open) in the suppression filtering logic if an ignore file entry lacks a required field. Furthermore, the widespread use of echo for JSON data processing poses a risk of data corruption due to backslash expansion; it is recommended to switch to printf for all data handling and add more robust validation for the suppression configuration file. Beyond these security aspects, there are also opportunities for improvement regarding code duplication, performance, and dead code.
scripts/helpers/suppression.sh
Outdated
| if [[ -n "$now" ]]; then | ||
| # With expiration checking | ||
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" ' | ||
| ($suppressions | map({(.id): .}) | add // {}) as $smap | |
There was a problem hiding this comment.
The filter_findings function uses a jq optimization that fails if any suppression entry in .clawpinch-ignore.json is missing the id field. Because clawpinch.sh runs with set -e, this failure causes the entire orchestrator to crash with a jq: error: Cannot use null as object key error. This can disrupt CI/CD pipelines or potentially lead to a bypass of security checks if the pipeline is configured to continue on tool failures.
Remediation: Update the jq filter to safely handle or skip entries missing an id field.
| ($suppressions | map({(.id): .}) | add // {}) as $smap | | |
| ($suppressions | map(select(.id != null) | {(.id): .}) | add // {}) as $smap | |
scripts/helpers/suppression.sh
Outdated
| # Use jq to split findings into active and suppressed | ||
| if [[ -n "$now" ]]; then | ||
| # With expiration checking | ||
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" ' |
There was a problem hiding this comment.
Using echo "$variable" to pass JSON data to jq is insecure because echo may interpret backslash sequences (like \n, \t, or \\) depending on the shell environment and configuration. If the security findings contain backslashes (common in file paths, regex patterns, or evidence), the data will be mangled before processing. This compromises the integrity of the security audit and can lead to corrupted reports or failed auto-fixes.
Remediation: Use printf '%s\n' "$variable" instead of echo for all data-passing operations.
| echo "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" ' | |
| printf '%s\n' "$findings" | jq -c --argjson suppressions "$_CLAWPINCH_SUPPRESSIONS" --arg now "$now" ' |
scripts/helpers/suppression.sh
Outdated
| # Fallback: all findings are active | ||
| local findings | ||
| findings="$(cat)" | ||
| echo "{\"active\": $findings, \"suppressed\": []}" |
There was a problem hiding this comment.
clawpinch.sh
Outdated
| if [[ "$SHOW_SUPPRESSED" -eq 1 ]]; then | ||
| # Mark suppressed findings with a "suppressed": true field before merging | ||
| MARKED_SUPPRESSED="$(echo "$SUPPRESSED_FINDINGS" | jq '[.[] | . + {suppressed: true}]')" | ||
| DISPLAY_FINDINGS="$(echo "$ACTIVE_FINDINGS" "$MARKED_SUPPRESSED" | jq -s 'def sev_order: if . == "critical" then 0 elif . == "warn" then 1 elif . == "info" then 2 elif . == "ok" then 3 else 4 end; .[0] + .[1] | sort_by(.severity | sev_order)')" |
There was a problem hiding this comment.
The sev_order jq function is duplicated here. It's also used to define SORTED_FINDINGS (lines 345-352). To improve maintainability and avoid potential inconsistencies, you could define this function once in a shell variable and reuse it in both places.
For example, you could define this near the top of the script:
JQ_SEV_ORDER_FUNC='def sev_order: if . == "critical" then 0 elif . == "warn" then 1 elif . == "info" then 2 elif . == "ok" then 3 else 4 end;'Then, you could use it here and for SORTED_FINDINGS.
| DISPLAY_FINDINGS="$(echo "$ACTIVE_FINDINGS" "$MARKED_SUPPRESSED" | jq -s 'def sev_order: if . == "critical" then 0 elif . == "warn" then 1 elif . == "info" then 2 elif . == "ok" then 3 else 4 end; .[0] + .[1] | sort_by(.severity | sev_order)')" | |
| DISPLAY_FINDINGS="$(echo "$ACTIVE_FINDINGS" "$MARKED_SUPPRESSED" | jq -s "${JQ_SEV_ORDER_FUNC} .[0] + .[1] | sort_by(.severity | sev_order)")" |
scripts/helpers/report.sh
Outdated
| id="$(echo "$json" | jq -r '.id // ""')" | ||
| severity="$(echo "$json" | jq -r '.severity // "info"')" | ||
| title="$(echo "$json" | jq -r '.title // ""')" | ||
| description="$(echo "$json" | jq -r '.description // ""')" | ||
| evidence="$(echo "$json" | jq -r '.evidence // ""')" | ||
| remediation="$(echo "$json" | jq -r '.remediation // ""')" | ||
| auto_fix="$(echo "$json" | jq -r '.auto_fix // ""')" | ||
| suppressed="$(echo "$json" | jq -r '.suppressed // false')" |
There was a problem hiding this comment.
This section makes multiple calls to jq to parse a single JSON object, which is inefficient as it spawns a new process for each field. You can refactor this to use a single jq call to extract all values at once, improving performance.
| id="$(echo "$json" | jq -r '.id // ""')" | |
| severity="$(echo "$json" | jq -r '.severity // "info"')" | |
| title="$(echo "$json" | jq -r '.title // ""')" | |
| description="$(echo "$json" | jq -r '.description // ""')" | |
| evidence="$(echo "$json" | jq -r '.evidence // ""')" | |
| remediation="$(echo "$json" | jq -r '.remediation // ""')" | |
| auto_fix="$(echo "$json" | jq -r '.auto_fix // ""')" | |
| suppressed="$(echo "$json" | jq -r '.suppressed // false')" | |
| # Read all values in one go to avoid multiple jq processes | |
| { | |
| read -r id | |
| read -r severity | |
| read -r title | |
| read -r description | |
| read -r evidence | |
| read -r remediation | |
| read -r auto_fix | |
| read -r suppressed | |
| } < <(echo "$json" | jq -r ' | |
| .id // "", | |
| .severity // "info", | |
| .title // "", | |
| .description // "", | |
| .evidence // "", | |
| .remediation // "", | |
| .auto_fix // "", | |
| .suppressed // false | |
| ') |
scripts/helpers/suppression.sh
Outdated
| is_suppressed() { | ||
| local check_id="$1" | ||
|
|
||
| # If no suppressions loaded, nothing is suppressed | ||
| if [[ "$_CLAWPINCH_SUPPRESSIONS" == "[]" ]]; then | ||
| return 1 | ||
| fi | ||
|
|
||
| # Require jq | ||
| if ! command -v jq &>/dev/null; then | ||
| return 1 | ||
| fi | ||
|
|
||
| # Get current timestamp in ISO 8601 format for expiration checking | ||
| local now | ||
| if command -v date &>/dev/null; then | ||
| if ! now="$(date -u +'%Y-%m-%dT%H:%M:%SZ' 2>/dev/null)"; then | ||
| now="" | ||
| fi | ||
| else | ||
| now="" | ||
| fi | ||
|
|
||
| # Check if the ID is in suppressions and not expired | ||
| local result | ||
| if [[ -n "$now" ]]; then | ||
| # With expiration checking | ||
| result="$(echo "$_CLAWPINCH_SUPPRESSIONS" | jq -r --arg id "$check_id" --arg now "$now" ' | ||
| map(select(.id == $id)) | | ||
| if length > 0 then | ||
| .[0] | | ||
| if .expires then | ||
| if .expires > $now then "suppressed" else "expired" end | ||
| else | ||
| "suppressed" | ||
| end | ||
| else | ||
| "active" | ||
| end | ||
| ' 2>/dev/null)" | ||
| else | ||
| # Without expiration checking (no date command or failed to get timestamp) | ||
| result="$(echo "$_CLAWPINCH_SUPPRESSIONS" | jq -r --arg id "$check_id" ' | ||
| if (map(select(.id == $id)) | length > 0) then | ||
| "suppressed" | ||
| else | ||
| "active" | ||
| end | ||
| ' 2>/dev/null)" | ||
| fi | ||
|
|
||
| [[ "$result" == "suppressed" ]] | ||
| } |
…ove dead code - Replace echo with printf for JSON construction in suppression.sh (security) - Add null check for suppression entries missing id field to prevent jq crash - Remove unused is_suppressed() function - Consolidate 8 separate jq calls into single call with read in print_finding() - Extract duplicated sev_order jq function into reusable JQ_SEV_ORDER_FUNC variable Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All round 3 review comments have been addressed in bbe77b0:
@gemini-code-assist @greptile-apps Ready for re-review. |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable finding suppression mechanism. However, a significant security regression was identified: the new JSON parsing method in scripts/helpers/report.sh is vulnerable to newline injection. This could allow attackers to manipulate the display of security findings, potentially hiding critical vulnerabilities by marking them as suppressed. Additionally, a high-severity bug in multi-line finding detail parsing could lead to corrupted output, and there are opportunities to remove dead code and simplify logic. Addressing the newline injection vulnerability and other identified issues is crucial for the feature's integrity and robustness.
scripts/helpers/report.sh
Outdated
| local id severity title description evidence remediation auto_fix suppressed | ||
| { | ||
| read -r id | ||
| read -r severity | ||
| read -r title | ||
| read -r description | ||
| read -r evidence | ||
| read -r remediation | ||
| read -r auto_fix | ||
| read -r suppressed | ||
| } < <(echo "$json" | jq -r ' | ||
| .id // "", | ||
| .severity // "info", | ||
| .title // "", | ||
| .description // "", | ||
| .evidence // "", | ||
| .remediation // "", | ||
| .auto_fix // "", | ||
| .suppressed // false | ||
| ') |
There was a problem hiding this comment.
The print_finding function in scripts/helpers/report.sh is vulnerable to newline injection. The current method of using a single jq call and read -r to parse multiple fields is not robust. If fields like description, evidence, or remediation contain newline characters, read will misinterpret the data, leading to incorrect parsing and a malformed report. This is a critical security concern as an attacker could inject newlines to manipulate the suppressed flag, potentially hiding critical vulnerabilities by forcing them to display as [SUPPRESSED] in the report. Reverting to individual jq calls for each field extraction will ensure data integrity and prevent this vulnerability.
| local id severity title description evidence remediation auto_fix suppressed | |
| { | |
| read -r id | |
| read -r severity | |
| read -r title | |
| read -r description | |
| read -r evidence | |
| read -r remediation | |
| read -r auto_fix | |
| read -r suppressed | |
| } < <(echo "$json" | jq -r ' | |
| .id // "", | |
| .severity // "info", | |
| .title // "", | |
| .description // "", | |
| .evidence // "", | |
| .remediation // "", | |
| .auto_fix // "", | |
| .suppressed // false | |
| ') | |
| local id severity title description evidence remediation auto_fix suppressed | |
| id="$(echo "$json" | jq -r '.id // ""')" | |
| severity="$(echo "$json" | jq -r '.severity // "info"')" | |
| title="$(echo "$json" | jq -r '.title // ""')" | |
| description="$(echo "$json" | jq -r '.description // ""')" | |
| evidence="$(echo "$json" | jq -r '.evidence // ""')" | |
| remediation="$(echo "$json" | jq -r '.remediation // ""')" | |
| auto_fix="$(echo "$json" | jq -r '.auto_fix // ""')" | |
| suppressed="$(echo "$json" | jq -r '.suppressed // false')" |
scripts/helpers/interactive.sh
Outdated
| local fix_mark | ||
| if [[ -n "$f_auto_fix" ]]; then | ||
| fix_mark=" ${_CLR_OK}✓${_CLR_RST} " | ||
| else | ||
| fix_mark=" ─ " | ||
| fi |
There was a problem hiding this comment.
scripts/helpers/suppression.sh
Outdated
| if command -v date &>/dev/null; then | ||
| if ! now="$(date -u +'%Y-%m-%dT%H:%M:%SZ' 2>/dev/null)"; then | ||
| now="" | ||
| fi | ||
| else | ||
| now="" | ||
| fi |
There was a problem hiding this comment.
The logic to get the current timestamp can be simplified. The nested if and explicit setting of now="" on failure is more verbose than necessary. You can achieve the same result more concisely by capturing the output of date and using || true to prevent script exit on failure.
| if command -v date &>/dev/null; then | |
| if ! now="$(date -u +'%Y-%m-%dT%H:%M:%SZ' 2>/dev/null)"; then | |
| now="" | |
| fi | |
| else | |
| now="" | |
| fi | |
| now="$(date -u +'%Y-%m-%dT%H:%M:%SZ' 2>/dev/null || true)" |
scripts/helpers/interactive.sh
Outdated
| local fix_mark | ||
| if [[ -n "$f_auto_fix" ]]; then | ||
| fix_mark=" ${_CLR_OK}✓${_CLR_RST} " | ||
| else | ||
| fix_mark=" ─ " | ||
| fi |
There was a problem hiding this comment.
Unused variable — fix_mark is set but never used.
| local fix_mark | |
| if [[ -n "$f_auto_fix" ]]; then | |
| fix_mark=" ${_CLR_OK}✓${_CLR_RST} " | |
| else | |
| fix_mark=" ─ " | |
| fi | |
| # Fix indicator (fixed-width, no color padding issues) |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/helpers/interactive.sh
Line: 254:259
Comment:
Unused variable — `fix_mark` is set but never used.
```suggestion
# Fix indicator (fixed-width, no color padding issues)
```
How can I resolve this? If you propose a fix, please make it concise.…ad code, simplify timestamp - report.sh: Revert consolidated jq/read to individual jq calls per field to prevent newline injection manipulating the suppressed flag (security-high) - interactive.sh: Remove unused fix_mark variable (dead code) - suppression.sh: Simplify timestamp logic — replace nested if with single date call using || true fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
All 4 review comments addressed in commit 3b7a7d3:
|
| # Load suppressions if not already loaded | ||
| if [[ "$_CLAWPINCH_SUPPRESSIONS" == "[]" ]] && [[ -f "$ignore_file" ]]; then | ||
| load_suppressions "$ignore_file" | ||
| fi |
There was a problem hiding this comment.
Suppressions can stay empty
filter_findings() only calls load_suppressions when _CLAWPINCH_SUPPRESSIONS == "[]". That means a valid ignore file with an empty suppressions: [] is indistinguishable from "not loaded", and it will re-load every call. Worse, if load_suppressions fails (invalid JSON) it returns 1 but leaves _CLAWPINCH_SUPPRESSIONS as [], so filtering still proceeds with an empty map (silently treating everything as active) even though the caller expects suppressions to apply. Consider tracking a separate "loaded" flag, and/or make filter_findings fail/propagate when load_suppressions returns non-zero so the user gets consistent behavior.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/helpers/suppression.sh
Line: 68:71
Comment:
**Suppressions can stay empty**
`filter_findings()` only calls `load_suppressions` when `_CLAWPINCH_SUPPRESSIONS == "[]"`. That means a *valid* ignore file with an empty `suppressions: []` is indistinguishable from "not loaded", and it will re-load every call. Worse, if `load_suppressions` fails (invalid JSON) it returns 1 but leaves `_CLAWPINCH_SUPPRESSIONS` as `[]`, so filtering still proceeds with an empty map (silently treating everything as active) even though the caller expects suppressions to apply. Consider tracking a separate "loaded" flag, and/or make `filter_findings` fail/propagate when `load_suppressions` returns non-zero so the user gets consistent behavior.
How can I resolve this? If you propose a fix, please make it concise.| if $s.expires and $s.expires <= $now then | ||
| .active += [$f] | ||
| else | ||
| .suppressed += [$f + {suppression: ($s | del(.id))}] | ||
| end | ||
| else |
There was a problem hiding this comment.
Expiration compares strings
Expiration handling uses if $s.expires and $s.expires <= $now where both are strings. This only works reliably if expires is always normalized to the same ISO-8601 format (YYYY-MM-DDTHH:MM:SSZ). If a user supplies a valid ISO timestamp with an offset (e.g. 2026-02-10T12:00:00-05:00) or fractional seconds, lexicographic compare can misclassify expiration. To avoid incorrect reactivation, parse to epoch seconds (e.g. fromdateiso8601) and compare numerically.
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/helpers/suppression.sh
Line: 98:103
Comment:
**Expiration compares strings**
Expiration handling uses `if $s.expires and $s.expires <= $now` where both are strings. This only works reliably if `expires` is always normalized to the same ISO-8601 format (`YYYY-MM-DDTHH:MM:SSZ`). If a user supplies a valid ISO timestamp with an offset (e.g. `2026-02-10T12:00:00-05:00`) or fractional seconds, lexicographic compare can misclassify expiration. To avoid incorrect reactivation, parse to epoch seconds (e.g. `fromdateiso8601`) and compare numerically.
How can I resolve this? If you propose a fix, please make it concise.
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: scripts/helpers/report.sh
Line: 913:919
Comment:
**Suppressed count not passed**
`print_summary_animated()` accepts 6 args and calls `print_summary` without forwarding the new `suppressed` (7th) parameter. In `clawpinch.sh`, you now call `print_summary_animated ... "$count_suppressed"`, but `report.sh` drops it, so the "Suppressed: N findings" line will never render (and the 7th arg is silently ignored). Update `print_summary_animated` to accept/pass the 7th arg to `print_summary`.
How can I resolve this? If you propose a fix, please make it concise. |
Allow users to suppress specific findings by check ID, with optional expiration dates and justification comments. Suppressions are stored in .clawpinch-ignore.json (similar to .eslintignore). Suppressed findings are still scanned but reported separately as 'suppressed' rather than counting toward severity totals. A --show-suppressed flag reveals them.